Adds support for "SameSite" cookie property #1246

kdckrs · 2020-10-02T09:45:32Z

Description (*)

This PR adds support to set the SameSite cookie property and is compatible with both the new setcookie signature (>= PHP 7.3) and the old one (< PHP 7.3).

By default the property is set to "None". The value can be changed in the admin-panel under System -> Configuration -> Web -> Session Cookie Management -> Same-Site

Related Pull Requests

/

Fixed Issues (if relevant)

Manual testing scenarios (*)

Open the console i(Network-tab) in your browser and be ready to inspect the Set-cookie header
Navigate to your shop to OpenMage shop
Confirm that the Set-cookie header contains "SameSite=None"
In another tab, go to the admin panel, go to "System -> Configuration -> Web -> Session Cookie Management -> Same-Site" and update the value to "Strict". (Flush cache...)
Go to your initial tab and refresh the page
Confirm that the Set-cookie header contains "SameSite=Strict"

Questions or comments

Open for feedback, first contribution to OpenMage ;)

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All automated tests passed successfully (all builds are green)

# Conflicts: # app/code/core/Mage/Core/Model/Cookie.php # app/code/core/Mage/Core/etc/system.xml # app/locale/en_US/Mage_Core.csv

rvelhote · 2020-10-08T17:30:43Z

This looks good from the commit. I will test this later.

In app/code/core/Mage/Core/Model/Cookie.php there is a delete method that that uses setcookie. Should we use the same procedure as the set method? (i.e. check for PHP version and pass the samesite parameter if applicable).

kdckrs · 2020-10-09T07:18:04Z

This looks good from the commit. I will test this later.

In app/code/core/Mage/Core/Model/Cookie.php there is a delete method that that uses setcookie. Should we use the same procedure as the set method? (i.e. check for PHP version and pass the samesite parameter if applicable).

For consistency one could say it would also be nice to have the same code in the "public function delete()", although the presence here isn't as critical as in the "public function renew()". If I can find some time today I will try and make this more complete.

rvelhote · 2020-10-09T11:35:00Z

Another upgrade that would be great is the ability to specify a different policy for the backend and the frontend. Perhaps this is mostly useful for the session cookie. For example I would configure the backend to have Strict and for the frontend Lax or None.

nimasan · 2020-10-12T09:24:54Z

Hello,

I reviewed the code and I wonder if we did have to force $secure to true if same site is None because of the specification : https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite#SameSiteNone_requires_Secure

app/code/core/Mage/Core/Model/Cookie.php

* SameSite consistency for renew and delete functions

kdckrs · 2020-10-16T19:17:59Z

This looks good from the commit. I will test this later.

In app/code/core/Mage/Core/Model/Cookie.php there is a delete method that that uses setcookie. Should we use the same procedure as the set method? (i.e. check for PHP version and pass the samesite parameter if applicable).

I have made some changes. The delete function now re-uses the "set function", only with forced "null" values for cookie value and period. (Same way the renew function was already implemented).

kdckrs · 2020-10-16T19:18:51Z

Hello,

I reviewed the code and I wonder if we did have to force $secure to true if same site is None because of the specification : https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite#SameSiteNone_requires_Secure

I have reviewed the specifications you have linked and you are indeed correct. I have made the required changes to make sure the secure flag is enforced of the SameSite is set to None.

Flyingmana · 2020-10-20T20:23:15Z

sorry, had no time to review this yet, but I saw M2 did release something related. Did someone already have a look at what they did?

app/code/core/Mage/Adminhtml/Model/System/Config/Source/Cookie/SameSite.php

kdckrs · 2020-12-02T14:40:39Z

Any update guys?

rvelhote · 2020-12-10T12:09:12Z

@kdckrs We finally were able to test this PR in our development environment and it worked as expected. We will deploy into production next week and report back.

nimasan · 2020-12-10T19:39:03Z

Feedback about experience.

We used your solution in production on our website since almost 2 months.
It seems to work as expected.

Thx for it.

kdckrs · 2020-12-29T13:40:37Z

@Flyingmana I saw there was a new release without this PR, any chance it will be included in the next one? We have been using this in a production environment for a while now and everything seems to be working correctly. (Others have mentioned this too, see previous comments)

Flyingmana · 2020-12-29T17:10:37Z

lets see, comments from @kkrieger85 and @nimajneb-torram are resolved.
@rvelhote and @nimajneb-torram have tested it.
Should be fine, so I will merge it for the next release.

colinmollenhour · 2020-12-29T17:11:24Z

The code looks good but the current behavior (no SameSite flag) is equivalent to "Lax" but the PR makes "None" the default. This has the effect of actually reducing security and also potentially breaking non-https sites (although these really shouldn't remain anyway). I propose making Lax the default.

github-actions · 2020-12-29T17:12:31Z

Unit Test Results

1 files 1 suites 0s ⏱️
0 tests 0 ✔️ 0 💤 0 ❌
2 runs 2 ✔️ 0 💤 0 ❌

Results for commit 26b9eee.

ansgarbecker · 2021-01-11T16:29:11Z

In the backend, on General > Web, Magento's autoloader attempts to load Mage_Adminhtml_Model_System_Config_Source_Cookie_Samesite , while the class file is named "SameSite.php" (with an upper "Site" in it). The file is not auto-loaded on case sensitive filesystems, and causes wild errors.
I'd recommend to rename the file (and class) Unix-friendly to "Samesite.php".

*Edit: Not sure if that's worth a bug report, just shout if so.

kdckrs · 2021-01-11T20:33:15Z

@ansgarbecker you are right! Fixing!

Edit: PR #1390 has been created for this ;)

kdckrs added 5 commits October 2, 2020 11:05

Support SameSite cookie property (fixes issue #414)

95cf20b

Support SameSite cookie property (fixes issue #414)

7c60f03

Merge remote-tracking branch 'origin/1.9.4.x' into 1.9.4.x

f78d0d0

# Conflicts: # app/code/core/Mage/Core/Model/Cookie.php # app/code/core/Mage/Core/etc/system.xml # app/locale/en_US/Mage_Core.csv

Open in new tab

ea687e1

Default fallback to None just in case someone forgets to clear cache

8a745ff

github-actions bot added Component: Adminhtml Relates to Mage_Adminhtml Component: Core Relates to Mage_Core translations Relates to app/locale labels Oct 2, 2020

nimasan suggested changes Oct 13, 2020

View reviewed changes

app/code/core/Mage/Core/Model/Cookie.php Outdated Show resolved Hide resolved

kdckrs added 4 commits October 16, 2020 20:49

* Force secure if SameSite None

64523f3

* SameSite consistency for renew and delete functions

Code formatting #OCD :)

da9b688

PHP 7.0 compatibility

52ef9a4

Fix pipeline errors

1567dc6

kkrieger85 reviewed Oct 22, 2020

View reviewed changes

app/code/core/Mage/Adminhtml/Model/System/Config/Source/Cookie/SameSite.php Outdated Show resolved Hide resolved

Removed incorrect copyright docs

650b2b3

fand0rin mentioned this pull request Nov 16, 2020

23389: Cookie - add support SameSite attribute (push from lts repo) BudsiesApp/magento-mirror#43

Merged

Flyingmana merged commit 26b9eee into OpenMage:1.9.4.x Dec 29, 2020

kdckrs mentioned this pull request Jan 11, 2021

Fix class name and filename for case sensitive filesystems #1390

Merged

3 tasks

kiatng mentioned this pull request Jan 4, 2023

New branching rules (2023) for OpenMage's main repo OpenMage/rfcs#6

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for "SameSite" cookie property #1246

Adds support for "SameSite" cookie property #1246

kdckrs commented Oct 2, 2020 •

edited

Loading

rvelhote commented Oct 8, 2020

kdckrs commented Oct 9, 2020

rvelhote commented Oct 9, 2020

nimasan commented Oct 12, 2020

kdckrs commented Oct 16, 2020

kdckrs commented Oct 16, 2020

Flyingmana commented Oct 20, 2020

kdckrs commented Dec 2, 2020

rvelhote commented Dec 10, 2020

nimasan commented Dec 10, 2020

kdckrs commented Dec 29, 2020

Flyingmana commented Dec 29, 2020

colinmollenhour commented Dec 29, 2020

github-actions bot commented Dec 29, 2020

ansgarbecker commented Jan 11, 2021 •

edited

Loading

kdckrs commented Jan 11, 2021 •

edited

Loading

Adds support for "SameSite" cookie property #1246

Adds support for "SameSite" cookie property #1246

Conversation

kdckrs commented Oct 2, 2020 • edited Loading

Description (*)

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

Questions or comments

Contribution checklist (*)

rvelhote commented Oct 8, 2020

kdckrs commented Oct 9, 2020

rvelhote commented Oct 9, 2020

nimasan commented Oct 12, 2020

kdckrs commented Oct 16, 2020

kdckrs commented Oct 16, 2020

Flyingmana commented Oct 20, 2020

kdckrs commented Dec 2, 2020

rvelhote commented Dec 10, 2020

nimasan commented Dec 10, 2020

kdckrs commented Dec 29, 2020

Flyingmana commented Dec 29, 2020

colinmollenhour commented Dec 29, 2020

github-actions bot commented Dec 29, 2020

Unit Test Results

ansgarbecker commented Jan 11, 2021 • edited Loading

kdckrs commented Jan 11, 2021 • edited Loading

kdckrs commented Oct 2, 2020 •

edited

Loading

ansgarbecker commented Jan 11, 2021 •

edited

Loading

kdckrs commented Jan 11, 2021 •

edited

Loading